-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new encoder for client and server #158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gosh this is fantastic imo! Some comments, and I am sure @yoshuawuyts may also have some.
|
||
self.finalize_headers()?; | ||
let mut headers = self.request.iter().collect::<Vec<_>>(); | ||
headers.sort_unstable_by_key(|(h, _)| if **h == HOST { "0" } else { h.as_str() }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that we'd be better off using some kind of order-stable hashmap rather that doing a sort here, since this sort could be O(n log(n))
. Not sure if a good one exists though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah… this logic also probably belongs in http-types, but as mentioned elsewhere i think that should be a distinct move from this change
bytes_read += len; | ||
} | ||
loop { | ||
self.state = match self.state { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is really neat!
|
||
/// like ready! but early-returns the Poll<Result<usize>> early in all situations other than Ready(Ok(0)) | ||
#[macro_export] | ||
macro_rules! read_to_end { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems useful generally tbh
// Else continue encoding | ||
self.encode_fixed_body(cx, buf) | ||
if self.response.header(DATE).is_none() { | ||
let date = fmt_http_date(SystemTime::now()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not critical, but this should probably be cached, Node.js does it to the minute: https://github.com/nodejs/node/blob/4f0f2e713848817d49a4105a0736db2dfeed061e/lib/internal/http.js#L19-L29
(I may be willing to that in a follow-up pr.)
byte-pool = "0.2.1" | ||
lazy_static = "1.4.0" | ||
futures-core = "0.3.1" | ||
log = "0.4" | ||
pin-project = "1.0.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in theory, pin-project-lite 0.2 has enum pinning, but i couldn't figure out how to get it to work correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch is fascinating, and so good. Very excited for the simplifications this PR bring. In conjunction with the more robust test suite this is incredible work all around!
Co-authored-by: Yoshua Wuyts <[email protected]>
this is based on #157. the changes in this pr are d80934d